-
-
Notifications
You must be signed in to change notification settings - Fork 194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split all parameters of a function application over multiple lines if they exceed page width #929
Conversation
@nojaf What is your first impression of this implementation? I am not quite sure about some changes I had to make to some tests. Also, what should we do if the parameters do not exceed page width if they were in one line, but they are split over multiple lines in the original and have comments at EOL? I was thinking to keep them split over multiple lines in that case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Bobface , thanks for this PR. I personally like the outcome.
Some tests are a bit weird though.
I hope my remarks are useful, let me know if you have any further questions.
src/Fantomas.Tests/ElmishTests.fs
Outdated
(span [ ClassName "has-text-weight-semibold" ] [ | ||
ofInt fullMatchedLength | ||
]) | ||
tokenDetailRow "FullMatchedLength" (span [ ClassName "has-text-weight-semibold" ] [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit weird as the span block is multiline.
So I would expect it to be more like
tokenDetailRow
"FullMatchedLength"
(span [ ...
])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the problem here is the following:
When the long expression is checked the printer tries to put all parameters in one line. Further down in the recursive calls where the array is printed it is detected that the page width will be exceeded if the array is printed in one line so it is split.
That satisfies the maximum width for the long expression again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might need your help on what's the best way to fix that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code now makes
tokenDetailRow
"FullMatchedLength"
(span [ ClassName "has-text-weight-semibold" ] [
ofInt fullMatchedLength
])
of it. This is correct behaviour in my opinion. The function call doesn't fit on the remainders of the line because of the arguments doesn't fit. This makes sense.
I think the exception here will be lambda's, if you have those, you might want to keep the original behaviour. See next comment.
src/Fantomas.Tests/RecordTests.fs
Outdated
Opt.valueWith "new value" | ||
[ "fourth" | ||
"ssssssssssssssssssssssssssssssssssssssssssssssssssss" ] ]) ] | ||
Opt.valueWith "new value" [ "fourth" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same remark, the last part is multiline
src/Fantomas/CodePrinter.fs
Outdated
+> unindent)) | ||
fun ctx -> | ||
(ifElseCtx | ||
(exceedsWidth ctx.Config.PageWidth |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use expressionFitsOnRestOfLine
instead here.
I would also create separate values for the short and long expression path.
Something like:
fun ctx ->
let shortExpression = ...
let longExpression = ...
expressionFitsOnRestOfLine shortExpression longExpression ctx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you might want to also check for isCompExpr
and use the existing expression in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to use expressionFitsOnRestOfLine
however it directly goes into "short expression mode" and splits even very short apps over multiple lines in some cases. Is that intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you try it in this PR? What you are describing sounds a bit weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have just pushed the version with expressionFitsOnRestOfLine
so you can see the failing tests where even short expressions are newlined. I assume shortExpressionWithFallback
which is used by expressionFitsOnRestOfLine
is the culprit (see the comment):
let private shortExpressionWithFallback (shortExpression: Context -> Context) (fallbackExpression) maxWidth startColumn (ctx: Context) =
// if the context is already inside a ShortExpression mode and tries to figure out if the expression will go over the page width,
// we should try the shortExpression in this case.
match ctx.WriterModel.Mode with
| ShortExpression infos when (List.exists (fun info -> info.ConfirmedMultiline || info.IsTooLong ctx.Config.PageWidth ctx.Column) infos) ->
ctx
| _ -> ....
3c81b03
to
be541d6
Compare
src/Fantomas/CodePrinter.fs
Outdated
+> unindent)))) | ||
|
||
//ifElseCtx (fun ctx -> exceedsWidth ctx.Config.PageWidth longExpression ctx || parametersMustBeNewlinedBecauseOfTriviaContent es ctx) shortExpression longExpression ctx | ||
ifElseCtx (fun ctx -> parametersMustBeNewlinedBecauseOfTriviaContent es ctx) shortExpression (expressionFitsOnRestOfLine longExpression shortExpression) ctx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expressionFitsOnRestOfLine longExpression shortExpression
that doesn't really make much sense.
The function expressionFitsOnRestOfLine
will first try the longExpression and use the short one as fallback.
Those parameters should be swapped.
I also think you don't always want this behaviour, it will depend on the es
of | App(e, es)
.
If one expression in es
is a lambda for example I don't think you necessarily want the suggested formatting of putting every argument on one line.
For example:
let foo =
Array.map (fun x ->
// some multiline code
x + x) [| 8;9;6;5 |]
I don't think you want to make
let foo =
Array.map
(fun x ->
// some multiline code
x + x)
[| 8;9;6;5 |]
of it.
src/Fantomas/CodePrinter.fs
Outdated
// c | ||
// | ||
// This has to be split over multiple lines to preserve the line comment | ||
let parametersMustBeNewlinedBecauseOfTriviaContent es ctx = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what unit test did you introduce this? I would assume that a comment in the expression will force expressionFitsOnRestOfLine
to take the fallback route so I'm not quite sure this is necessary here.
Hey @Bobface, what do you think of nojaf@6a28f6f. The same for lambda's, we want the |
b4444b8
to
d28d6e9
Compare
… they exceed page width
d28d6e9
to
e5e4b7d
Compare
@nojaf This PR is now using your implementation for the
Some of the parameters for |
Hey @Bobface , yes that test looks a bit weird but it still makes sense I think: So when there is a multiline string in place, the old formatting will be used. let appNlnFun e =
match e with
| MultilineString _
| Lambda _
| Paren (Lambda _)
| Paren (MatchLambda _) -> id
| _ -> autoNlnIfExpressionExceedsPageWidth so for the MultilineString we don't add a newline, yet for all the other parameter we add a newline when they reach the page width threshold. @jindraivanek would you mind reviewing this as well please? |
Fixes #907 by putting every parameter of a function application on a newline if the function application exceeds
PageWidth
.cc @knocte